perf: add canary patterns to skip statistics during bulk operations#150
perf: add canary patterns to skip statistics during bulk operations#150gonzalesedwin1123 merged 2 commits into19.0from
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements bulk membership creation for programs and cycles using raw SQL with ON CONFLICT DO NOTHING to optimize performance and handle duplicate entries. It introduces 'canary' context flags to bypass expensive field recomputations during bulk processes, along with refresh methods to synchronize statistics upon completion. Review feedback highlights a missing refresh method for registrant statistics which could lead to stale data, and questions the redundancy of the cycle statistics refresh method for non-stored fields.
| if self.env.context.get("skip_registrant_statistics"): | ||
| return |
There was a problem hiding this comment.
The skip_registrant_statistics context flag allows bypassing the recomputation of store=True fields such as program_membership_count. However, unlike the spp.program and spp.cycle models, no corresponding refresh method is provided for res.partner to recompute these statistics after bulk operations. This will result in stale data remaining in the database if the flag is used or if raw SQL is employed.
Please consider adding a refresh_registrant_statistics() method to the res.partner inheritance and calling it in the appropriate completion handlers (e.g., in mark_import_as_done within eligibility_manager.py).
| def refresh_statistics(self): | ||
| """Refresh all cycle statistics after bulk operations. | ||
|
|
||
| Call this after raw SQL inserts that bypass ORM dependency tracking | ||
| (e.g. bulk_create_memberships with skip_duplicates=True). | ||
| """ | ||
| self._compute_members_count() | ||
| self._compute_entitlements_count() | ||
| self._compute_total_entitlements_count() |
There was a problem hiding this comment.
The refresh_statistics method appears to be redundant in its current implementation. The fields it attempts to refresh (members_count, entitlements_count, and total_entitlements_count) are all store=False and do not implement the "canary" skip logic found in other models.
Since these fields are computed on-demand and the underlying relation caches are correctly invalidated in the managers (e.g., via cycle.invalidate_recordset(['cycle_membership_ids'])), they will naturally reflect the correct values upon the next access without an explicit refresh call. If the intention was to optimize these fields for bulk operations, they should be made store=True and implement the skip logic, similar to has_members in spp.program.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #150 +/- ##
=======================================
Coverage 71.35% 71.36%
=======================================
Files 932 932
Lines 54769 54794 +25
=======================================
+ Hits 39080 39103 +23
- Misses 15689 15691 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Add context flags (skip_registrant_statistics, skip_program_statistics) that allow bulk operation callers to suppress expensive computed field recomputation. Add refresh_beneficiary_counts() on spp.program and refresh_statistics() on spp.cycle to recompute once at completion. Also replace bool(rec.program_membership_ids) with SQL EXISTS in _compute_has_members to avoid loading the full membership recordset.
gonzalesedwin1123
force-pushed
the
worktree-perf+phase8-canary-patterns
branch
from
1c16e0c to
8c49d83
Compare
2 participants
Summary
skip_registrant_statistics,skip_program_statistics) that allow bulk operation callers to suppress expensive computed field recomputationrefresh_beneficiary_counts()onspp.programandrefresh_statistics()onspp.cycleto recompute all statistics once after bulk operations completebool(rec.program_membership_ids)with SQL query in_compute_has_membersto avoid loading full membership recordsetmark_import_as_doneandmark_check_eligibility_as_doneChanges
programs.py: Addskip_program_statisticsguard to_compute_has_members, replace with SQL query, addrefresh_beneficiary_counts()cycle.py: Addrefresh_statistics()methodregistrant.py: Addskip_registrant_statisticsguard to 4 computed methods (membership count, entitlements, cycles, in-kind)cycle_manager_base.py: Usecycle.refresh_statistics()inmark_import_as_doneeligibility_manager.py: Useprogram.refresh_beneficiary_counts()inmark_import_as_donetest_canary_patterns.py(new): 9 tests covering canary flags, SQL-based has_members, and refresh methodsContext
Phase 8 of 9 in the
spp_programsperformance optimization effort. Rebased on current 19.0. Version bumped to 19.0.2.0.9.Test plan
./scripts/test_single_module.sh spp_programs— 615 tests, 0 failurespre-commit run --files <changed_files>— all checks pass